Cs 10652/introduce the cardthumbnail inside cardinfofield#4341
Cs 10652/introduce the cardthumbnail inside cardinfofield#4341
Conversation
Preview deployments |
24941df to
5d71e37
Compare
Host Test Results 1 files + 1 1 suites +1 2h 26m 4s ⏱️ + 2h 26m 4s For more details on these errors, see this check. Results for commit fa904a6. ± Comparison against base commit dffc745. ♻️ This comment has been updated with latest results. |
Realm Server Test Results 1 files ±0 1 suites ±0 13m 57s ⏱️ -4s For more details on these failures, see this check. Results for commit fa904a6. ± Comparison against base commit dffc745. ♻️ This comment has been updated with latest results. |
f681244 to
485ed43
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8747621238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/base/image-file-def.gts
Outdated
| static atom: BaseDefComponent = Atom; | ||
| static fitted: BaseDefComponent = Fitted; | ||
| } | ||
| export { ImageDef as default } from './card-api'; |
There was a problem hiding this comment.
Re-export ImageDef as named export from image-file-def
image-file-def previously exposed ImageDef as a named export, but this shim now exports only default. Any existing card/module that still does import { ImageDef } from 'https://cardstack.com/base/image-file-def' will fail module loading with a missing export error, which breaks the stated backward-compatibility goal for the shim and can break existing realms at runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR consolidates FileDef, ImageDef, and NumberField into packages/base/card-api.gts (to break cyclic deps), updates canonical FileDef module references from file-api to card-api, and introduces a new cardThumbnail relationship on CardInfo to support a linked thumbnail image.
Changes:
- Move
FileDef,ImageDef, andNumberFieldimplementations intocard-api.gts, convertingfile-api.gts,image-file-def.gts, andnumber.gtsinto shim/re-export modules. - Add
CardInfo.cardThumbnail(links toImageDef) and updateCardDef.cardThumbnailURLto prefer the linked image URL with fallback to the legacy string field. - Update query filters / fixtures / index assumptions and test expectations to use
card-apias the canonicalFileDefmodule reference.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/realm-index-query-engine.ts | Default FileDef adoptsFrom module updated to card-api. |
| packages/runtime-common/constants.ts | baseFileRef now points at card-api instead of file-api. |
| packages/realm-server/tests/search-prerendered-test.ts | Test query filter updated to card-api FileDef. |
| packages/realm-server/tests/realm-endpoints/search-test.ts | Test filters updated to card-api FileDef. |
| packages/realm-server/tests/prerendering-test.ts | Prerender test fixture updated to card-api FileDef. |
| packages/realm-server/tests/indexing-test.ts | Indexed doc fixture updated to card-api FileDef. |
| packages/host/tests/unit/services/render-service-test.ts | File meta adoptsFrom fixture updated to card-api. |
| packages/host/tests/integration/store-test.gts | File meta adoptsFrom fixtures updated to card-api. |
| packages/host/tests/integration/resources/search-test.ts | Search filters updated to card-api FileDef. |
| packages/host/tests/integration/resources/search-data-test.ts | Data search filter updated to card-api FileDef. |
| packages/host/tests/integration/realm-querying-test.gts | Querying tests now use card-api FileDef refs. |
| packages/host/tests/integration/realm-indexing-test.gts | Expected dependency references updated for moved implementations/new imports. |
| packages/host/tests/integration/components/prerendered-card-search-test.gts | Query filter updated to card-api FileDef. |
| packages/host/tests/integration/components/file-def-serialization-test.gts | Serialization expectation updated to card-api FileDef. |
| packages/host/tests/helpers/interact-submode-setup.gts | Dynamic import adjusted for image-file-def default export. |
| packages/host/tests/cards/file-query-card.gts | Query card’s filter updated to card-api FileDef. |
| packages/experiments-realm/image-def-playground.gts | Import ImageDef now comes from card-api. |
| packages/experiments-realm/file-search-playground.gts | Query filters updated to card-api FileDef. |
| packages/base/webp-image-def.gts | Updated ImageDef import to match new default export shim. |
| packages/base/svg-image-def.gts | Updated ImageDef import to match new default export shim. |
| packages/base/png-image-def.gts | Updated ImageDef import to match new default export shim. |
| packages/base/number.gts | Shim now re-exports NumberField + helpers from card-api. |
| packages/base/jpg-image-def.gts | Updated ImageDef import to match new default export shim. |
| packages/base/image-file-def.gts | Converted to shim re-exporting ImageDef from card-api. |
| packages/base/gif-image-def.gts | Updated ImageDef import to match new default export shim. |
| packages/base/file-api.gts | Converted to shim re-exporting FileDef and related helpers/types from card-api. |
| packages/base/default-templates/card-info.gts | Adds cardThumbnail editor UI and adjusts thumbnail placeholder logic. |
| packages/base/card-api.gts | New canonical home for FileDef, ImageDef, NumberField, and adds CardInfo.cardThumbnail + updated cardThumbnailURL behavior. |
| packages/base/avif-image-def.gts | Updated ImageDef import to match new default export shim. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/base/image-file-def.gts
Outdated
| static atom: BaseDefComponent = Atom; | ||
| static fitted: BaseDefComponent = Fitted; | ||
| } | ||
| export { ImageDef as default } from './card-api'; |
There was a problem hiding this comment.
image-file-def.gts used to export a named ImageDef class, but now the shim only re-exports it as the module default. That breaks existing consumers that import { ImageDef } from https://cardstack.com/base/image-file-def. To preserve backward compatibility, re-export ImageDef as a named export as well (and optionally keep the default export).
| export { ImageDef as default } from './card-api'; | |
| export { ImageDef, ImageDef as default } from './card-api'; |
| @field cardThumbnailURL = contains(MaybeBase64Field, { | ||
| computeVia: function (this: CardDef) { | ||
| return this.cardInfo.cardThumbnailURL; | ||
| return this.cardInfo.cardThumbnail?.url ?? this.cardInfo.cardThumbnailURL; |
There was a problem hiding this comment.
The new cardThumbnailURL computeVia reads this.cardInfo.cardThumbnail?.url. Accessing a linksTo field triggers lazilyLoadLink() when the relationship is not loaded, which will start an async store.loadFileMetaDocument() fetch even though this computeVia can’t await it and will usually fall back to cardThumbnailURL anyway. This can introduce unnecessary background loads/N+1 behavior during serialization/prerendering. Consider avoiding the linksTo getter here (e.g., inspect the raw stored relationship value first, or only use cardThumbnail?.url when the linked ImageDef is already loaded).
| return this.cardInfo.cardThumbnail?.url ?? this.cardInfo.cardThumbnailURL; | |
| let rawCardThumbnail = getDataBucket(this.cardInfo).get('cardThumbnail'); | |
| let loadedThumbnailURL = | |
| rawCardThumbnail && | |
| typeof rawCardThumbnail === 'object' && | |
| 'url' in rawCardThumbnail && | |
| typeof rawCardThumbnail.url === 'string' | |
| ? rawCardThumbnail.url | |
| : undefined; | |
| return loadedThumbnailURL ?? this.cardInfo.cardThumbnailURL; |
There was a problem hiding this comment.
this is a really bad suggestion. it should be ok to pull on a linksTo field in the computed. we will only lazily load it if this field is included in a template, which is exactly the behavior that we want. we should NEVER reach directly into the low level data bucket for a card. that is not a userland API. ignore this review comment
There was a problem hiding this comment.
Agreed with it. Sometimes Copilot gives feedback that feels a bit aggressive, which is not the behavior we want.
burieberry
left a comment
There was a problem hiding this comment.
To keep the amount of code added to the card-api to a minimum, you can place any large component or helpers/etc that doesn't cause cyclic error outside of the card-api. You'll see we're doing this with a bunch of templates in /default-templates.
linear: https://linear.app/cardstack/issue/CS-10652/introduce-the-cardthumbnail-inside-cardinfofield
Fix cyclic deps:
Demo:
Screen.Recording.2026-04-09.at.5.33.24.PM.mov